Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: runtime deprecate SlowBuffer #55175

Merged

Conversation

RafaelGSS
Copy link
Member

SlowBuffer has been doc-deprecated for a while and I couldn't find why we didn't want to proceed with the deprecation cycle on this feature. Therefore, this PR runtime deprecates SlowBuffer.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Sep 30, 2024
@RafaelGSS RafaelGSS added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 30, 2024
@RafaelGSS
Copy link
Member Author

Tagging @nodejs/tsc as it's a semver-major that might land on Node.js 23.

lib/buffer.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 30, 2024
@aduh95 aduh95 added the needs-citgm PRs that need a CITGM CI run. label Sep 30, 2024
@RedYetiDev RedYetiDev added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deprecation sounds good to me. Let's run CITGM anyway and please add a test as well.

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Sep 30, 2024

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@RafaelGSS
Copy link
Member Author

CITGM results:

FAILURE: 79 failures in 3478 not present in 3479


┌────────────────────────┬─────────────────────┬─────────────────────┬────────────────────────────────┬───────────────────────┬─────────────────────┬───────────────────┬──────────────────────────┬──────────────────┬───────────────┬───────────────────┬───────────────┬───────────────┬───────────────┬───────────────────────┬──────────────────┬───────────────┬──────────────┐
│ (index)                │ 0                   │ 1                   │ 2                              │ 3                     │ 4                   │ 5                 │ 6                        │ 7                │ 8             │ 9                 │ 10            │ 11            │ 12            │ 13                    │ 14               │ 15            │ 16           │
├────────────────────────┼─────────────────────┼─────────────────────┼────────────────────────────────┼───────────────────────┼─────────────────────┼───────────────────┼──────────────────────────┼──────────────────┼───────────────┼───────────────────┼───────────────┼───────────────┼───────────────┼───────────────────────┼──────────────────┼───────────────┼──────────────┤
│ fedora-last-latest-x64 │ '@yarnpkg/cli'      │ 'gulp-v5.0.0'       │ 'jest-v29.7.0'                 │ 'lru-cache-v11.0.1'   │ 'mime-v4.0.4'       │ 'nan-v2.20.0'     │ 'pino-v9.4.0'            │ 'weak-v1.0.1'    │ 'ws-v8.18.0'  │                   │               │               │               │                       │                  │               │              │
│ debian12-x64           │ '@yarnpkg/cli'      │ 'fastify-v5.0.0'    │ 'import-in-the-middle-v1.11.2' │ 'jest-v29.7.0'        │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'     │ 'nan-v2.20.0'            │ 'undici-v6.19.8' │ 'weak-v1.0.1' │ 'ws-v8.18.0'      │               │               │               │                       │                  │               │              │
│ osx11                  │ '@yarnpkg/cli'      │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'                  │ 'nan-v2.20.0'         │ 'undici-v6.19.8'    │ 'weak-v1.0.1'     │ 'ws-v8.18.0'             │                  │               │                   │               │               │               │                       │                  │               │              │
│ win-vs2022             │ 'resolve-v1.22.8'   │ 'weak-v1.0.1'       │                                │                       │                     │                   │                          │                  │               │                   │               │               │               │                       │                  │               │              │
│ fedora-latest-x64      │ '@hapi/shot-v6.0.1' │ '@yarnpkg/cli'      │ 'bcrypt-v5.1.1'                │ 'binary-split-v1.0.5' │ 'bl-v6.0.16'        │ 'inherits-v2.0.4' │ 'is-core-module-v2.15.1' │ 'jest-v29.7.0'   │ 'koa-v2.15.3' │ 'lodash-v4.17.21' │ 'mime-v4.0.4' │ 'nan-v2.20.0' │ 'pino-v9.4.0' │ 'prom-client-v15.1.3' │ 'pumpify-v2.0.1' │ 'weak-v1.0.1' │ 'ws-v8.18.0' │
│ rhel8-ppc64le          │ '@yarnpkg/cli'      │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'                  │ 'nan-v2.20.0'         │ 'weak-v1.0.1'       │ 'ws-v8.18.0'      │                          │                  │               │                   │               │               │               │                       │                  │               │              │
│ rhel8-x64              │ '@yarnpkg/cli'      │ 'jest-v29.7.0'      │ 'lru-cache-v11.0.1'            │ 'mime-v4.0.4'         │ 'nan-v2.20.0'       │ 'pino-v9.4.0'     │ 'weak-v1.0.1'            │ 'ws-v8.18.0'     │               │                   │               │               │               │                       │                  │               │              │
│ ubuntu2204-64          │ '@yarnpkg/cli'      │ 'jest-v29.7.0'      │ 'lru-cache-v11.0.1'            │ 'mime-v4.0.4'         │ 'nan-v2.20.0'       │ 'weak-v1.0.1'     │ 'ws-v8.18.0'             │                  │               │                   │               │               │               │                       │                  │               │              │
│ rhel8-s390x            │ '@yarnpkg/cli'      │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'                  │ 'weak-v1.0.1'         │ 'ws-v8.18.0'        │                   │                          │                  │               │                   │               │               │               │                       │                  │               │              │
│ alpine-latest-x64      │ 'jest-v29.7.0'      │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'                  │ 'nan-v2.20.0'         │ 'weak-v1.0.1'       │ 'ws-v8.18.0'      │                          │                  │               │                   │               │               │               │                       │                  │               │              │
│ aix72-ppc64            │ 'undici-v6.19.8'    │ 'weak-v1.0.1'       │                                │                       │                     │                   │                          │                  │               │                   │               │               │               │                       │                  │               │              │
└────────────────────────┴─────────────────────┴─────────────────────┴────────────────────────────────┴───────────────────────┴─────────────────────┴───────────────────┴──────────────────────────┴──────────────────┴───────────────┴───────────────────┴───────────────┴───────────────┴───────────────┴───────────────────────┴──────────────────┴───────────────┴──────────────┘
These modules failed in all platforms: weak-v1.0.1

But, looking at those errors, it doesn't seem related to this change, but to the main branch. I'll spin another CITGM for main and compare.

@RafaelGSS
Copy link
Member Author

@RafaelGSS
Copy link
Member Author

FAILURE: 7 failures in 3478 not present in 3483


┌────────────────────────┬────────────────────────────────┬─────────────────┬───────────────────────┬──────────────┬──────────────────┬───────────────────┐
│ (index)                │ 0                              │ 1               │ 2                     │ 3            │ 4                │ 5                 │
├────────────────────────┼────────────────────────────────┼─────────────────┼───────────────────────┼──────────────┼──────────────────┼───────────────────┤
│ fedora-last-latest-x64 │                                │                 │                       │              │                  │                   │
│ debian12-x64           │ 'import-in-the-middle-v1.11.2' │                 │                       │              │                  │                   │
│ osx11                  │                                │                 │                       │              │                  │                   │
│ win-vs2022             │                                │                 │                       │              │                  │                   │
│ fedora-latest-x64      │ '@hapi/shot-v6.0.1'            │ 'bcrypt-v5.1.1' │ 'binary-split-v1.0.5' │ 'bl-v6.0.16' │ 'fastify-v5.0.0' │ 'lodash-v4.17.21' │
│ rhel8-ppc64le          │                                │                 │                       │              │                  │                   │
│ rhel8-x64              │                                │                 │                       │              │                  │                   │
│ ubuntu2204-64          │                                │                 │                       │              │                  │                   │
│ rhel8-s390x            │                                │                 │                       │              │                  │                   │
│ alpine-latest-x64      │                                │                 │                       │              │                  │                   │
│ aix72-ppc64            │                                │                 │                       │              │                  │                   │
└────────────────────────┴────────────────────────────────┴─────────────────┴───────────────────────┴──────────────┴──────────────────┴───────────────────┘

It seems ok to land on main. @BridgeAR

@BridgeAR
Copy link
Member

BridgeAR commented Oct 4, 2024

@RafaelGSS could you please add a test for the deprecation? We have a couple of tests like that. Otherwise it's fine to land

@RafaelGSS
Copy link
Member Author

@RafaelGSS could you please add a test for the deprecation? We have a couple of tests like that. Otherwise it's fine to land

1817d2d.

@BridgeAR PTAL. I will do the last sync for v23 today and I'd like to have it included

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2024
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS force-pushed the runtime-deprecate-slow-buffer branch from 1817d2d to 446244a Compare October 10, 2024 13:48
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (10addb0) to head (0a56956).
Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55175   +/-   ##
=======================================
  Coverage   88.41%   88.41%           
=======================================
  Files         652      652           
  Lines      186864   186868    +4     
  Branches    36064    36061    -3     
=======================================
+ Hits       165217   165225    +8     
+ Misses      14888    14887    -1     
+ Partials     6759     6756    -3     
Files with missing lines Coverage Δ
lib/buffer.js 100.00% <100.00%> (ø)

... and 19 files with indirect coverage changes

@RafaelGSS
Copy link
Member Author

I'm not sure why lint is complaining. I'm not changing the files this is mentioning. Also, it doesn't fail locally. Can it be related to lint cache?

@richardlau
Copy link
Member

I'm not sure why lint is complaining. I'm not changing the files this is mentioning. Also, it doesn't fail locally. Can it be related to lint cache?

Since you pushed today, I suspect something has landed on main that has broken the linter -- maybe a conflict/race between something that changed the rules and a something else that didn't have those rules when it had its CI run?

@RafaelGSS
Copy link
Member Author

Reference in n

Looks like it was this PR #54160

@RafaelGSS RafaelGSS force-pushed the runtime-deprecate-slow-buffer branch from 446244a to 0a56956 Compare October 14, 2024 18:59
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2024
@nodejs-github-bot nodejs-github-bot merged commit 019efe1 into nodejs:main Oct 15, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 019efe1

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#55175
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants